-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the simple 4 axis stepper control respect the axis inversion settings in Configuration_prusa.h #1263
Conversation
Thanks exactly what I needed |
@PavelSindler This is a great bug-fix and i don't understand why Prusa didn't merged it yet. |
@PavelSindler @mkbel Any update on this pull request as it works great and fixes a hard coded issue/bug |
Well for some reason adding these modifications to the latest branch results in random segmentation faults and missing _PRI_LANG_SIGNATURE on MK2_rambo13a multi-language. This is very weird since it shouldn't affect the languages in any way, but it does. I'll try to fix the issue and make a pull request to your branch if I succeed fixing it. |
I can't reproduce this. And there is no way this could happen anyway. Sorry, but I think you're mistaken and have misattributed the problem to these changes. The problems you describe are what happens when you incorrectly build the multilanguage version. Are you building it using the PR-build.sh? You won't get a viable multilanguage firmware if you don't build it using the instructions specific to PR-build (only supported on linux and windows, and requires some additional setup. See the README.md). |
I also spent a lot of time to modify the FW in sm4.c to work with my Arduino RAMPS board. |
@metacollin Yes, you are right. Even without the modification it fails compiling some of the printers randomly. I am using PF-build.sh. I tried it again in a virtual machine and this time there was no problem. Maybe reinstalling git and the modifications to mingw will fix this. Thanks for pointing this out. |
@PavelSindler @mkbel @DRracer Is there any reason this PR didn't make it yet? I use it for months and just works great. |
I think that it might be very low priority as it doesn't actually have any impact on the actual Prusa printers. And honestly, I wouldn't really blame them. They're job is to support Prusa hardware, and this PR doesn't fix anything on Prusa hardware. But they are leaving it open so the fix is at least available to other people. shrug. |
@metacollin Their priority might be low for this one, but in my opinion it is a bug. Hardcode it in one part and use 'INVERT_?_DIR' every else isn't helping. Just imaging they decide to turn a motor for any reason, test its movement and it is working. But MBL may be off ... searching begins .... and that gets even harder when months/years past by and tons of commits are done. |
@wavexx please verify this PR in relation to your latest changes in sm4.* and twi.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested these changes on the latest MK3 branch and they work correctly. Just finished an XYZ calibration on a stock MK3S. It remains to be tested also on a MK2.5S. If that printer passes the calibration, then I say the PR is ready to be merged
#define ZDIR INVERT_Z_DIR:!INVERT_Z_DIR | ||
#define EDIR INVERT_E0_DIR:!INVERT_E0_DIR | ||
|
||
uint8_t dir_mask = 0x0F^(INVERT_X_DIR | (INVERT_Y_DIR << 1) | (INVERT_Z_DIR << 2) | (INVERT_E0_DIR << 3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is a non-constant variable that is declared globally, the compiler manages to optimize it completely because it is never written to it. So it works just as well as a define
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving after checking the expressions in both BOARD_RAMBO_MINI_1_0
and BOARD_EINSY_1_0a
Hi, I had heavily modified my MK2.5 printer by building a new, more rigid frame and swapping out the RAMBo mini for an Einsy. Part of all these modifications resulted in having to invert or not invert the axes differently from what the firmware would expect for a MK3 (or even a MK25 - I needed to invert all of my axes except for the extruder, opposite the MK25 and just plain wrong for the MK3).
This wouldn't be a problem, and it wasn't, except in one special case: the new "optical"/rastering XYZ calibration. I peeked at the code and realized that the simple 4 axis controller, in sm4.h and sm4.c and used exclusively for this new XYZ calibration, didn't actually respect the axis inversion or non inversion flags in the Configuration_prusa.h header.
I modified sm4.c such that it would respect these flags, and did my best to preserve the low cycle counts required for the various functions. There is an unavoidable timing increase in the sm4_set_dir function due to the comparison, but it doesn't seem to impact the calibration any a meaningful way. In fact, it makes the raster scanning part of the calibration noticeably quieter and smoother (I assume there are a large number of z-axis direction changes that are taking place and the added cycle time moves the direction changes a bit further from some resonance or something like that). The total time it takes to scan the region around a calibration point is certainly slightly longer, but the difference is so small that I couldn't notice any difference in the time required.
I know this pull request will have no effect for any of the supported printer configurations, but it would be extremely helpful to users like to modify their printers.
Plus, if or when the day arrives that Prusa Research engineers are developing some new product or tweaking an old one or whatever else, and they need to invert an axis... Well, this pull request ensures that their change doesn't immediately break XYZ calibration, and much potential frustration is now avoided.
Even if it isn't actually pulled into the code, at least it will be here in case someone finds it useful as a reference in the future.
Thank you for taking the time to look at this request!